FETCH .. WITH TIES issue detection #2111
Conversation
makalaaneesh
requested review from
sanyamsinghal and
priyanshi-yb
and removed request for
sanyamsinghal
yb-voyager/cmd/constants.go
Outdated
| @@ -214,6 +214,7 @@ const ( | |||
| BEFORE_FOR_EACH_ROW_TRIGGERS_ON_PARTITIONED_TABLE_FEATURE = "BEFORE ROW triggers on Partitioned tables" | |||
| PK_UK_CONSTRAINT_ON_COMPLEX_DATATYPES_FEATURE = "Primary / Unique key constraints on complex datatypes" | |||
| REGEX_FUNCTIONS_FEATURE = "Regex Functions" | |||
| FETCH_WITH_TIES_FEATURE = "FETCH .. WITH TIES" | |||
There was a problem hiding this comment.
nit: maybe "FETCH .. WITH TIES Clause"?
| func (d *SelectStmtDetector) getSelectStmtFromProto(msg protoreflect.Message) (*pg_query.SelectStmt, error) { | ||
| protoMsg := msg.Interface().(proto.Message) | ||
| selectStmtNode, ok := protoMsg.(*pg_query.SelectStmt) | ||
| if !ok { | ||
| return nil, fmt.Errorf("failed to cast %s to %s", queryparser.PG_QUERY_SELECTSTMT_NODE, queryparser.PG_QUERY_SELECTSTMT_NODE) | ||
| } | ||
| return selectStmtNode, nil | ||
| } |
There was a problem hiding this comment.
Lets add this function inhelper_protomsg.go
| } | ||
| // checks if a SelectStmt node uses a FETCH clause with TIES | ||
| // https://www.postgresql.org/docs/13/sql-select.html#SQL-LIMIT | ||
| if selectStmtNode.LimitOption == pg_query.LimitOption_LIMIT_OPTION_WITH_TIES { |
There was a problem hiding this comment.
define this limit option const pg_query.LimitOption_LIMIT_OPTION_WITH_TIES in queryparser
After these two changes (this and the above one), you won't need to use the pg_query package. Because we already have queryparser pkg for parser related stuff. So I think we should not use the pg_query directly here.
There was a problem hiding this comment.
Got it. Don't think we can completely avoid this because the return type would be a pg_query.SelectStmt. But we probably don't need to have an explicit dependency.
But broadly speaking, agree with not preferring to use pg_query directly here. (as much as possilble).
There was a problem hiding this comment.
Yes, as much as possible we should avoid using it directly in the queryissue.
| func (d *SelectStmtDetector) Detect(msg protoreflect.Message) error { | ||
| if queryparser.GetMsgFullName(msg) == queryparser.PG_QUERY_SELECTSTMT_NODE { | ||
| selectStmtNode, err := d.getSelectStmtFromProto(msg) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // checks if a SelectStmt node uses a FETCH clause with TIES | ||
| // https://www.postgresql.org/docs/13/sql-select.html#SQL-LIMIT | ||
| if selectStmtNode.LimitOption == pg_query.LimitOption_LIMIT_OPTION_WITH_TIES { | ||
| d.limitOptionWithTiesDetected = true | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I am thinking can we stick to one of the approaches (protomsg) as a primary approach, and use other one where its significantly making things easy, other code might become a bit messy.
There is a overhead of conversion also comes.
Here in this case i guess something like GetStringField() or GetEnumField() we already have, can be used to get pg_query.LimitOption_LIMIT_OPTION_WITH_TIES
There was a problem hiding this comment.
I see your point about consistency. But why, in general, do you prefer accessing fields of a message via protoreflect.Message rather than structs directly?
There is a overhead of conversion also comes.
I doubt there is significant overhead. It's just type assertion.
There was a problem hiding this comment.
Merged this for now, since it's been pending for a while and I had to resolve a lot of conflicts!
But let's discuss this nonetheless.
There was a problem hiding this comment.
But why, in general, do you prefer accessing fields of a message via protoreflect.Message rather than structs directly?
@makalaaneesh
Couple of reasons:
The field name we use to fetch from the protomsg is exactly the same as visible in a parse tree output.
In case of structs, you need to figureout the struct field which gives you the required info(sligtly different named)
Consistency here is another reason.
| @@ -515,3 +515,65 @@ func TestRegexFunctionsIssue(t *testing.T) { | |||
| } | |||
|
|
|||
| } | |||
|
|
|||
| func TestWithTies(t *testing.T) { | |||
There was a problem hiding this comment.
nit: TestFetchWithTiesInSelect ?
Describe the changes in this pull request
https://yugabyte.atlassian.net/browse/DB-14221
Describe if there are any user-facing changes
How was this pull request tested?
Does your PR have changes that can cause upgrade issues?